chore(API): Refactor authority check functions in API #189

szabodaniel995 · 2024-04-21T16:52:52Z

User description

Description

Refactored the authority checking functionalities into a service
Renamed the lookup functions adhering to the requested new names
Annotated the input of the lookup functions with a common interface
Added requested new lookup functionality

Fixes #180

Developer's checklist

My PR follows the style guidelines of this project
I have performed a self-check on my work

If changes are made in the code:

I have followed the coding guidelines
My changes in code generate no new warnings
My changes are breaking another fix/feature of the project
I have added test cases to show that my feature works
I have added relevant screenshots in my PR
There are no UI/UX issues

Documentation Update

This PR requires an update to the documentation at docs.keyshade.xyz
I have made the necessary updates to the documentation, or no documentation changes are required.

Type

enhancement, bug_fix

Description

Introduced AuthorityCheckerService to centralize authority checks across various services.
Refactored ApprovalService, EnvironmentService, ProjectService, SecretService, VariableService, and WorkspaceService to use the new AuthorityCheckerService.
Removed old authority check functions, replacing them with calls to AuthorityCheckerService.
Updated CommonModule to include AuthorityCheckerService in its providers and exports.

Changes walkthrough

Relevant files

Enhancement

approval.service.ts `Refactor Approval Service to Use AuthorityCheckerService` apps/api/src/approval/service/approval.service.ts Refactored authority checks to use `AuthorityCheckerService` instead of standalone functions. Updated method calls to use new service methods for authority validation.	+16/-14
authority-checker.service.ts `Add AuthorityCheckerService for Centralized Authority Checks` apps/api/src/common/authority-checker.service.ts Introduced a new service `AuthorityCheckerService` to handle authority checks. Implemented methods to validate user authority over workspaces, projects, environments, variables, and secrets.	+380/-0
environment.service.ts `Refactor Environment Service to Use AuthorityCheckerService` apps/api/src/environment/service/environment.service.ts Refactored to use `AuthorityCheckerService` for authority checks. Removed old authority check functions and replaced them with service method calls.	+39/-33
project.service.ts `Integrate AuthorityCheckerService in Project Service` apps/api/src/project/service/project.service.ts Integrated `AuthorityCheckerService` for authority validations. Removed old project authority check functions.	+39/-33
secret.service.ts `Utilize AuthorityCheckerService in Secret Service` apps/api/src/secret/service/secret.service.ts Utilized `AuthorityCheckerService` for authority checks in secret management. Removed old secret authority check functions.	+61/-58
variable.service.ts `Adopt AuthorityCheckerService in Variable Service` apps/api/src/variable/service/variable.service.ts Adopted `AuthorityCheckerService` for authority checks in variable management. Removed old variable authority check functions.	+64/-58
workspace.service.ts `Integrate AuthorityCheckerService in Workspace Service` apps/api/src/workspace/service/workspace.service.ts Integrated `AuthorityCheckerService` for workspace-related authority checks. Removed old workspace authority check functions.	+86/-74

Configuration changes

common.module.ts `Update CommonModule to Include AuthorityCheckerService` apps/api/src/common/common.module.ts Added `AuthorityCheckerService` to the providers and exports of the CommonModule.	+3/-2

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-free · 2024-04-21T16:53:43Z

PR Description updated to latest commit (2935c5b)

codiumai-pr-agent-free · 2024-04-21T16:53:55Z

PR Review

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant refactoring and centralization of authority checking functions across multiple services. It requires a thorough understanding of the existing codebase and the changes to ensure that the new implementation correctly handles all the authority checks without introducing regressions or security issues.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The refactoring introduces a centralized `AuthorityCheckerService` which replaces multiple existing functions. If not all use cases are covered by the new service, it could lead to authorization bugs.
🔍 Possible issues	Performance Concern: The centralized authority checks could introduce a single point of failure and potential performance bottlenecks if not properly optimized, especially in high-load environments.
🔒 Security concerns	No

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

codiumai-pr-agent-free · 2024-04-21T16:54:44Z

PR Code Suggestions

Category	Suggestions
Enhancement	Add error handling for database access in the `checkAuthorityOverWorkspace` function. Consider adding error handling for the `findUnique` and `findFirst` methods in the `checkAuthorityOverWorkspace` function to handle potential database errors more gracefully. apps/api/src/common/authority-checker.service.ts [36-40] -workspace = await prisma.workspace.findUnique({ - where: { - id: entity.id +try { + workspace = await prisma.workspace.findUnique({ + where: { + id: entity.id + } + }); + if (!workspace) { + throw new NotFoundException(`Workspace with id ${entity.id} not found`); } -}) +} catch (error) { + throw new InternalServerErrorException('Database access error'); +}
	Improve the exception message for better debugging and user feedback. Use a more specific exception message in `checkAuthorityOverWorkspace` when the workspace is not found, including both `id` and `name` when available. apps/api/src/common/authority-checker.service.ts [54] -throw new NotFoundException(`Workspace with id ${entity.id} not found`) +throw new NotFoundException(`Workspace not found with id: ${entity.id} or name: ${entity.name}`)
	Refactor authority checks into a single reusable method to reduce code duplication. Refactor the repeated code for authority checks into a separate method within the `SecretService` class. This method could take parameters like `entityId`, `authorityType`, and `entityType` to generalize the authority check process, reducing code duplication. apps/api/src/secret/service/secret.service.ts [67-84] -const project = - await this.authorityCheckerService.checkAuthorityOverProject({ +const project = await this.checkAuthority('project', projectId, Authority.CREATE_SECRET); +... +const environment = await this.checkAuthority('environment', environmentId, Authority.READ_ENVIRONMENT); +... +private async checkAuthority(entityType: string, entityId: string, authorityType: Authority): Promise<any> { + return await this.authorityCheckerService[`checkAuthorityOver${entityType}`]({ userId: user.id, - entity: { id: projectId }, - authority: Authority.CREATE_SECRET, + entity: { id: entityId }, + authority: authorityType, prisma: this.prisma }); -... -const environment = - await this.authorityCheckerService.checkAuthorityOverEnvironment({ - userId: user.id, - entity: { id: environmentId }, - authority: Authority.READ_ENVIRONMENT, - prisma: this.prisma - }); +}
Maintainability	Refactor permission checks into a separate method to reduce code duplication. Refactor the repeated code for checking permissions in `checkAuthorityOverWorkspace` into a separate method to improve code maintainability and reduce duplication. apps/api/src/common/authority-checker.service.ts [64-70] -if ( - !permittedAuthorities.has(authority) && - !permittedAuthorities.has(Authority.WORKSPACE_ADMIN) -) { - throw new UnauthorizedException( - `User ${userId} does not have the required authorities to perform the action` - ); -} +this.verifyPermissions(permittedAuthorities, authority, userId);
Maintainability	Use destructuring for the `user` object to improve code clarity. To improve code readability and maintainability, consider destructuring the `user` object to directly access the `id` property when it is used multiple times. apps/api/src/secret/service/secret.service.ts [69] -userId: user.id, +const { id: userId } = user; +... +userId: userId,
Bug	Add null checks for `entity` to prevent potential runtime errors. To prevent potential null reference errors, add null checks for `entity` before accessing `entity.id` or `entity.name` in the `checkAuthorityOverWorkspace` function. apps/api/src/common/authority-checker.service.ts [35-39] -if (entity?.id) { +if (entity && entity.id) { workspace = await prisma.workspace.findUnique({ where: { id: entity.id } }); }
Bug	Add a null check for the `project` variable after its asynchronous assignment. To ensure that the `project` variable is always defined before it's used, especially in asynchronous operations, consider adding a null check right after its assignment. This can prevent runtime errors in subsequent code that assumes the project exists. apps/api/src/secret/service/secret.service.ts [67-73] const project = await this.authorityCheckerService.checkAuthorityOverProject({ userId: user.id, entity: { id: projectId }, authority: Authority.CREATE_SECRET, prisma: this.prisma }); +if (!project) { + throw new Error('Project not found or access denied'); +}
Performance	Combine conditions into a single query to optimize database access. Optimize the database query in `checkAuthorityOverWorkspace` by combining the conditions for `id` and `name` into a single query using `OR` logic, reducing the need for separate queries. apps/api/src/common/authority-checker.service.ts [35-46] -if (entity?.id) { - workspace = await prisma.workspace.findUnique({ - where: { - id: entity.id - } - }); -} else { - workspace = await prisma.workspace.findFirst({ - where: { - name: entity?.name - } - }); -} +workspace = await prisma.workspace.findFirst({ + where: { + OR: [ + { id: entity?.id }, + { name: entity?.name } + ] + } +});
Best practice	Add error handling for asynchronous authority check operations. Consider using a try-catch block around the asynchronous call to `checkAuthorityOverProject` to handle potential exceptions, such as network issues or database errors, which could otherwise lead to unhandled promise rejections. apps/api/src/secret/service/secret.service.ts [67-73] -const project = - await this.authorityCheckerService.checkAuthorityOverProject({ +let project; +try { + project = await this.authorityCheckerService.checkAuthorityOverProject({ userId: user.id, entity: { id: projectId }, authority: Authority.CREATE_SECRET, prisma: this.prisma - }) + }); +} catch (error) { + // Handle error appropriately + console.error('Failed to check authority over project', error); +}
	Add error handling for the `getDefaultEnvironmentOfProject` call to manage potential failures gracefully. To avoid potential issues with unhandled promise rejections, consider adding error handling for the `getDefaultEnvironmentOfProject` call. This function is likely to interact with external systems or databases, which might fail. apps/api/src/secret/service/secret.service.ts [87] -environment = await getDefaultEnvironmentOfProject(projectId, this.prisma) +try { + environment = await getDefaultEnvironmentOfProject(projectId, this.prisma); +} catch (error) { + console.error('Failed to get default environment:', error); + // Handle error appropriately +}
	Make the `authorityCheckerService` property immutable by adding the `readonly` modifier. Consider using TypeScript's `readonly` modifier for `authorityCheckerService` to prevent reassignment and ensure the service's immutability within the class. apps/api/src/environment/service/environment.service.ts [31] -public authorityCheckerService: AuthorityCheckerService +public readonly authorityCheckerService: AuthorityCheckerService
	Enhance encapsulation by changing the `authorityCheckerService` access modifier to `private`. To ensure encapsulation and data integrity, change the access modifier of `authorityCheckerService` from `public` to `private` since it does not seem to be used outside of this class. apps/api/src/socket/change-notifier.socket.ts [54] -public authorityCheckerService: AuthorityCheckerService +private authorityCheckerService: AuthorityCheckerService
	Improve class encapsulation by setting `authorityCheckerService` to `private`. It's a good practice to ensure that all injected services in a constructor are either `private` or `protected` unless they need to be exposed, which enhances encapsulation. Change `authorityCheckerService` to `private`. apps/api/src/workspace-role/service/workspace-role.service.ts [32] -public authorityCheckerService: AuthorityCheckerService +private authorityCheckerService: AuthorityCheckerService
	Ensure the immutability of `authorityCheckerService` by using the `readonly` modifier. To maintain consistency and ensure that services are not reassigned after their initial assignment, add the `readonly` modifier to `authorityCheckerService`. apps/api/src/event/service/event.service.ts [10] -public authorityCheckerService: AuthorityCheckerService +public readonly authorityCheckerService: AuthorityCheckerService
	Change `authorityCheckerService` to `private` to enhance data encapsulation. For better encapsulation and to prevent external modifications, consider changing `authorityCheckerService` from `public` to `private` since it does not appear to be used outside of this class. apps/api/src/approval/service/approval.service.ts [47] -public authorityCheckerService: AuthorityCheckerService +private authorityCheckerService: AuthorityCheckerService
	Mock the `AuthorityCheckerService` in the test setup to ensure isolation and reliability of the tests. Ensure that the `AuthorityCheckerService` is mocked in the testing module to prevent actual authority checks during testing, which can lead to unpredictable test outcomes and dependencies on external factors. apps/api/src/environment/controller/environment.controller.spec.ts [14] -providers: [EnvironmentService, PrismaService, AuthorityCheckerService] +providers: [EnvironmentService, PrismaService, { provide: AuthorityCheckerService, useValue: mockDeep<AuthorityCheckerService>() }]

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

…in-API

rajdip-b · 2024-04-21T16:55:23Z

I'll review this as soon as I can!

rajdip-b

Went through the PR, had a few suggestions that I have listed on the lines itself. Had a few general suggestions which I'm listing in here.

I feel we should use the existing file-based approach rather than class. This is because the code is maintainable that way.
We will need to add the members check that to each and every entity - secret, variable, environment, project and workspace. Mentioned the details in the review.
For entities like secret, variable, environment and project that do not have a members attribute, we will need to drill into the parent. Eg: secret will have secret.environment.project.workspace.

apps/api/src/approval/service/approval.service.ts

apps/api/src/common/authority-checker.service.ts

git

apps/api/src/common/authority-checker.service.ts

szabodaniel995 · 2024-04-21T20:05:04Z

Went through the PR, had a few suggestions that I have listed on the lines itself. Had a few general suggestions which I'm listing in here.

I feel we should use the existing file-based approach rather than class. This is because the code is maintainable that way.

We will need to add the members check that to each and every entity - secret, variable, environment, project and workspace. Mentioned the details in the review.

For entities like secret, variable, environment and project that do not have a members attribute, we will need to drill into the parent. Eg: secret will have secret.environment.project.workspace.

Thank you for the review

Resolved most
See my comment on your last suggestion

I'm not sure the existing tests cover example cases for the results of these queries. It might be a good idea to write some e2e tests simulating real business use cases

rajdip-b

Agreed on your suggestions. We do need to add a few tests to cover these scenarios. But there are two blockers:

Our test suites need to be refactored a bit.
It falls out of the scope of this PR.

I will be happy to open up another issue in case you feel like fixing it. But that might take some time since I need to refactor the tests.

And, you will still need to make a few updates to the code!

you can move the custom logger that you implemented into the common module.
if not already present, add a @ Global() to the common module to export it's providers globally
Remove all provider imports(if any) that you have made for authority checker and logger.
Add readonly to all occurrences of private authorityCheckerservice

szabodaniel995 · 2024-04-22T07:51:26Z

Agreed on your suggestions. We do need to add a few tests to cover these scenarios. But there are two blockers:

Our test suites need to be refactored a bit.

It falls out of the scope of this PR.

I will be happy to open up another issue in case you feel like fixing it. But that might take some time since I need to refactor the tests.

And, you will still need to make a few updates to the code!

you can move the custom logger that you implemented into the common module.

if not already present, add a @ Global() to the common module to export it's providers globally

Remove all provider imports(if any) that you have made for authority checker and logger.

Add readonly to all occurrences of private authorityCheckerservice

Updated the code.

Thank you for the opportunity, but I believe that these tests should be created by a person with good insight into the business logic and in possession of some real use cases.

sonarcloud · 2024-04-22T11:42:34Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

rajdip-b · 2024-04-22T11:46:35Z

Thanks for working on this man! Much appreciated.

szabodaniel995 · 2024-04-22T12:22:16Z

Thanks for working on this man! Much appreciated.

Glad to have helped.

Thank you :-)

## [1.3.0](v1.2.0...v1.3.0) (2024-05-12) ### 🚀 Features * Add approval support ([#158](#158)) ([e09ae60](e09ae60)) * **api:** Add configuration live update support ([#181](#181)) ([f7d6684](f7d6684)) * **api:** Add feature to export data of a workspace ([#152](#152)) ([46833aa](46833aa)) * **api:** Add Integration support ([#203](#203)) ([f1ae87e](f1ae87e)) * **api:** Add note to [secure] and variable ([#151](#151)) ([2e62351](2e62351)) * **api:** Add OAuth redirection and polished authentication ([#212](#212)) ([d2968bc](d2968bc)) * **api:** Add support for storing and managing variables ([#149](#149)) ([963a8ae](963a8ae)) * **api:** Added GitLab OAuth ([#188](#188)) ([4d3bbe4](4d3bbe4)) * **api:** Added validation for reason field ([#190](#190)) ([90b8ff2](90b8ff2)) * **api:** Create default workspace on user's creation ([#182](#182)) ([3dc0c4c](3dc0c4c)) * **api:** Reading `port` Dynamically ([#170](#170)) ([fd46e3e](fd46e3e)) * **auth:** Add Google OAuth ([#156](#156)) ([cf387ea](cf387ea)) * **web:** Added waitlist ([#168](#168)) ([1084c77](1084c77)) * **web:** Landing revamp ([#165](#165)) ([0bc723b](0bc723b)) ### 🐛 Bug Fixes * **web:** alignment issue in “Collaboration made easy” section ([#178](#178)) ([df5ca75](df5ca75)) * **workspace:** delete duplicate tailwind config ([99d922a](99d922a)) ### 📚 Documentation * add contributor list ([f37569a](f37569a)) * Add integration docs ([#204](#204)) ([406ddb7](406ddb7)) * Added integration docs to gitbook summary ([ab37530](ab37530)) * **api:** Add swagger docs of API key controller ([#167](#167)) ([2910476](2910476)) * **api:** Add swagger docs of User Controller ([#166](#166)) ([fd59522](fd59522)) * fix typo in environment-variables.md ([#163](#163)) ([48294c9](48294c9)) * Remove supabase from docs ([#169](#169)) ([eddbce8](eddbce8)) * **setup:** replace NX with Turbo in setup instructions ([#175](#175)) ([af8a460](af8a460)) * Update README.md ([b59f16b](b59f16b)) * Update running-the-api.md ([177dbbf](177dbbf)) * Update running-the-api.md ([#193](#193)) ([3d5bcac](3d5bcac)) ### 🔧 Miscellaneous Chores * Added lockfile ([60a3b9b](60a3b9b)) * Added lockfile ([6bb512c](6bb512c)) * **api:** Added type inference and runtime validation to `process.env` ([#200](#200)) ([249e07d](249e07d)) * **api:** Fixed prisma script env errors ([#209](#209)) ([8762354](8762354)) * **API:** Refactor authority check functions in API ([#189](#189)) ([e9d710d](e9d710d)) * **api:** Refactor user e2e tests ([b38d45a](b38d45a)) * **ci:** Disabled api stage release ([97877c4](97877c4)) * **ci:** Update stage deployment config ([868a6a1](868a6a1)) * **codecov:** update api-e2e project coverage ([1e90d7e](1e90d7e)) * **dockerfile:** Fixed web dockerfile ([6134bb2](6134bb2)) * **docker:** Optimized web Dockerfile to reduct image size ([#173](#173)) ([444286a](444286a)) * **release:** Downgraded package version ([c173fee](c173fee)) * **release:** Fix failing release ([#213](#213)) ([40f64f3](40f64f3)) * **release:** Install pnpm ([1081bea](1081bea)) * **release:** Updated release commit ([b8958e7](b8958e7)) * **release:** Updated release commit ([e270eb8](e270eb8)) * Update deprecated husky Install command ([#202](#202)) ([e61102c](e61102c)) * Upgrade @million/lint from 0.0.66 to 0.0.73 ([#172](#172)) ([dd43ed9](dd43ed9)) * **web:** Updated fly memory config ([4debc66](4debc66)) ### 🔨 Code Refactoring * **api:** Made events central to workspace ([#159](#159)) ([9bc00ae](9bc00ae)) * **api:** Migrated to cookie based authentication ([#206](#206)) ([ad6911f](ad6911f)) * **monorepo:** Migrate from nx to turbo ([#153](#153)) ([88b4b00](88b4b00))

rajdip-b · 2024-05-12T17:15:06Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

szabodaniel995 added 2 commits April 21, 2024 18:22

refactor - Refactor authority check functions in API 180

dc0caa2

refactor - Refactor authority check functions in API 180

2935c5b

szabodaniel995 requested a review from rajdip-b as a code owner April 21, 2024 16:52

codiumai-pr-agent-free bot added type: enhancement New feature or request bug_fix labels Apr 21, 2024

codiumai-pr-agent-free bot added the Review effort [1-5]: 4 label Apr 21, 2024

Merge branch 'develop' into 180---Refactor-authority-check-functions-…

27353e1

…in-API

szabodaniel995 changed the title ~~180 - refactor authority check functions in api~~ refactor: 180 - refactor authority check functions in api Apr 21, 2024

Switch all remaining absolute imports to relative imports

6b83482

rajdip-b changed the title ~~refactor: 180 - refactor authority check functions in api~~ chore(API): Refactor authority check functions in API Apr 21, 2024

rajdip-b requested changes Apr 21, 2024

View reviewed changes

Review + moved custom logger into its own service

780a798

rajdip-b requested changes Apr 22, 2024

View reviewed changes

review2, moved logger service into common module

41cdf0d

removed CommonModule imports

a5525cf

rajdip-b approved these changes Apr 22, 2024

View reviewed changes

rajdip-b merged commit e9d710d into keyshade-xyz:develop Apr 22, 2024
5 checks passed

kriptonian1 pushed a commit that referenced this pull request Apr 24, 2024

chore(API): Refactor authority check functions in API (#189)

667737c

rajdip-b added the released label May 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(API): Refactor authority check functions in API #189

chore(API): Refactor authority check functions in API #189

szabodaniel995 commented Apr 21, 2024 •

edited

Loading

codiumai-pr-agent-free bot commented Apr 21, 2024

codiumai-pr-agent-free bot commented Apr 21, 2024

codiumai-pr-agent-free bot commented Apr 21, 2024

rajdip-b commented Apr 21, 2024

rajdip-b left a comment

szabodaniel995 commented Apr 21, 2024

rajdip-b left a comment •

edited

Loading

szabodaniel995 commented Apr 22, 2024

sonarcloud bot commented Apr 22, 2024

rajdip-b commented Apr 22, 2024

szabodaniel995 commented Apr 22, 2024

rajdip-b commented May 12, 2024

chore(API): Refactor authority check functions in API #189

chore(API): Refactor authority check functions in API #189

Conversation

szabodaniel995 commented Apr 21, 2024 • edited Loading

User description

Description

Developer's checklist

Documentation Update

Type

Description

Changes walkthrough

codiumai-pr-agent-free bot commented Apr 21, 2024

codiumai-pr-agent-free bot commented Apr 21, 2024

PR Review

codiumai-pr-agent-free bot commented Apr 21, 2024

PR Code Suggestions

rajdip-b commented Apr 21, 2024

rajdip-b left a comment

Choose a reason for hiding this comment

szabodaniel995 commented Apr 21, 2024

rajdip-b left a comment • edited Loading

Choose a reason for hiding this comment

szabodaniel995 commented Apr 22, 2024

sonarcloud bot commented Apr 22, 2024

Quality Gate passed

rajdip-b commented Apr 22, 2024

szabodaniel995 commented Apr 22, 2024

rajdip-b commented May 12, 2024

szabodaniel995 commented Apr 21, 2024 •

edited

Loading

rajdip-b left a comment •

edited

Loading